This repository was archived by the owner on Jul 24, 2024. It is now read-only.
Absorb sass-graph 2.2.5 into node-sass#3202
Closed
kiskoza wants to merge 2 commits intosass:masterfrom
Closed
Conversation
|
This pull request introduces 1 alert when merging 518afbb into e80d4af - view on LGTM.com new alerts:
|
Contributor
|
Thanks but we maintain both packages so we're happy to keep them separate. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi.
The current version of node-sass depends on sass-graph v2.2.5. Unfortunately that package has a known vulnerability reported by various audit tools. I already tried to open a PR to fix it (see xzyfer/sass-graph#117), but apart from an initial comment stating that the package is in maintenance mode, I haven't got too much response. There's an open issue for this in node-sass' repo too: #3190
Some of the comments are saying you should change node-sass to dart-sass, but other comments are saying that they have a performance issue with dart-sass, so it's not an option for them. One of the comments suggested we should merge sass-graph into node-sass and fix the problem in this repo. I tried to follow this path.
If you check the report closely, you can see that the reported vulnerability is related to sass-graph's command line binary, which was never used in node-sass. That means it should be enough to copy over sass-graph without the CLI tool.
I had to make these changes:
lib/sass-graph/*.js- copy over sass-graph's corelib/watcher.js- include the new file instead of a packagepackage.json- remove sass-graph and includescss-tokenizer. this one was the only relevant diff between sass-graph's dependencies and the packages already required for node-sasstest/sass-graph/*.*- copy over sass-graph's test. I changedrequire('chai').asserttorequire('assert').strictand fixed the path to find the right sass-graph filesI hope this PR will help expanding this project's lifetime and reduce the cost of maintenance in the future. Is there anything else I need to do?